feat(core): sets default chunk_size and sends buffer > chunk_size directly #4710

evenyag · 2024-06-07T15:33:52Z

What's changed

Currently, CompleteLayer sanitizes the chunk size when users call with_chunk() explicitly.
https://docs.rs/opendal/latest/opendal/raw/struct.OpWrite.html#method.with_chunk

If users don't provide a chunk size, the writer won't guarantee the body size is larger than the minimum multi-part upload size. It's easy to forget to set the correct chunk size for the writer.

This PR changes this behavior:

Uses write_multi_min_size or write_multi_align_size as the chunk size if users don't provide one.
The ChunkedWriter attempts to send the bs directly if it is larger than the chunk size
- It adds a new argument exact to the ChunkedWriter. If exact is true, the writer still ensures the upload body size is exactly the chunk size.
- Backends with write_multi_align_size will set exact to true.
- If users provide a chunk size, exact will also be true.

There is a breaking change to ChunkedWriter::new().

Reference

GreptimeTeam/greptimedb#4098 (comment)

evenyag · 2024-06-07T16:10:12Z

The docs CI for the main branch is broken.
https://github.com/apache/opendal/actions/workflows/docs.yml?query=branch%3Amain

I guess the failure of this check is unrelated to this PR.
https://github.com/apache/opendal/actions/runs/9419578266/job/25949653713?pr=4710

---- behavior::test_rename_nested ----
test panicked: read must succeed: Unexpected (persistent) at  => reader got too little data

Context:
   expect: 64912
   actual: 0

Xuanwo · 2024-06-07T16:26:21Z

Thanks for you PR, I will take a review this weekend 🙏

Xuanwo

Thanks a lot for your PR! This change affects our behavior of chunk in write process. Would you like to update the comments for chunk too? Like:

opendal/core/src/types/operator/operator.rs

Lines 918 to 932 in 78692ce

    
           /// ## `chunk` 
        
           /// 
        
           /// Set `chunk` for the writer. 
        
           /// 
        
           /// OpenDAL is designed to write files directly without chunking by default, giving users 
        
           /// control over the exact size of their writes and helping avoid unnecessary costs. 
        
           /// 
        
           /// This is not efficient for cases when users write small chunks of data. Some storage services 
        
           /// like `s3` could even return hard errors like `EntityTooSmall`. Besides, cloud storage services 
        
           /// will cost more money if we write data in small chunks. 
        
           /// 
        
           /// Set a good chunk size might improve the performance, reduce the API calls and save money. 
        
           /// 
        
           /// The following example will set the writer chunk to 8MiB. Only one API call will be sent at 
        
           /// `close` instead.

Xuanwo · 2024-06-10T13:24:20Z

core/src/raw/oio/write/chunked_write.rs

 pub struct ChunkedWriter<W: oio::Write> {
    inner: W,

    /// The size for buffer, we will flush the underlying storage at the size of this buffer.
    chunk_size: usize,
+    /// If `exact` is true, the size of the data written to the underlying storage is
+    /// exactly `chunk_size` bytes.
+    exact: bool,


Nice idea about introducing exact.

Xuanwo · 2024-06-10T13:26:25Z

core/src/raw/oio/write/chunked_write.rs

@@ -49,6 +55,12 @@ impl<W: oio::Write> oio::Write for ChunkedWriter<W> {
            self.buffer.advance(written);
        }

+        if !self.exact && bs.len() >= self.chunk_size && self.buffer.is_empty() {


We can calculate if bs.len() + self.buffer.len() >= self.chunk_size and write them at once. This could be helpful for cases like: write(1KiB), write(16MiB), write(1KiB).

evenyag · 2024-06-11T12:01:32Z

Would you like to update the comments for chunk too

Fixed in d6a0449

evenyag · 2024-06-11T12:31:53Z

Some tests failed. I'll take a look.

Xuanwo · 2024-06-12T03:09:44Z

core/src/layers/complete.rs

-        });
+                size
+            });
+        let exact = capability.write_multi_align_size.is_some();


Hi, I just noticed that exact doesn't behave as I expected. If user has configured one or write_multi_align_size is some, we should use exact. Some storage services requires to use the exactly the same chunk for uploading, like R2

Object part sizes must be at least 5MiB but no larger than 5GiB. All parts except the last one must be the same size.

Xuanwo · 2024-06-12T03:20:16Z

core/src/raw/oio/write/chunked_write.rs

            self.buffer.advance(written);
+            // We didn't sent `bs`.
+            return Ok(0);


Write returning 0 often been treated as an error WriteZero. It's better not to do this.

We can push only 1B in the buffer.

This case is extremely unusual, and almost no other service behaves this way. Those rules are just a safety measure. We can make it clear in the comments.

What about pushing written bytes to the buffer queue? We already sent written bytes so the queue should have "written" bytes space left.

I'll push a commit later.

Makes sense to me.

Xuanwo

Thanks a lot!

github-actions bot requested review from ClSlaid and silver-ymz June 7, 2024 15:34

github-actions bot added the releases-note/feat label Jun 7, 2024

evenyag force-pushed the feat/default-chunk branch from 0eaaa22 to f1ab650 Compare June 7, 2024 15:34

evenyag added 3 commits June 7, 2024 23:36

feat: use write_multi_min_size/align_size as default chunk size

f020b35

feat: send bytes directly if the bytes >= chunk size

dc53393

chore: update comment

0e38441

evenyag force-pushed the feat/default-chunk branch from f1ab650 to 0e38441 Compare June 7, 2024 15:36

evenyag marked this pull request as ready for review June 7, 2024 16:10

evenyag requested a review from Xuanwo as a code owner June 7, 2024 16:10

Xuanwo reviewed Jun 10, 2024

View reviewed changes

evenyag added 3 commits June 11, 2024 18:57

chore: Merge branch 'main' into feat/default-chunk

bc1994e

docs: update docs

d6a0449

feat: send provided buffer and the buffer queue together

94e52ec

evenyag requested a review from Xuanwo June 11, 2024 12:02

evenyag marked this pull request as draft June 11, 2024 12:32

evenyag added 3 commits June 11, 2024 21:32

test: test partial send

f5d28c9

fix: partial send

ea94621

chore: fix typo

f233378

evenyag marked this pull request as ready for review June 11, 2024 14:55

Xuanwo reviewed Jun 12, 2024

View reviewed changes

evenyag added 2 commits June 12, 2024 14:55

feat: always set exact flag if users set chunk

0e1fa56

feat: push n more bytes to queue after written n bytes

83312b3

Xuanwo approved these changes Jun 12, 2024

View reviewed changes

Xuanwo merged commit 27c6a77 into apache:main Jun 12, 2024
214 checks passed

evenyag deleted the feat/default-chunk branch June 13, 2024 03:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core): sets default chunk_size and sends buffer > chunk_size directly #4710

feat(core): sets default chunk_size and sends buffer > chunk_size directly #4710

evenyag commented Jun 7, 2024 •

edited

Loading

evenyag commented Jun 7, 2024 •

edited

Loading

Xuanwo commented Jun 7, 2024

Xuanwo left a comment

Xuanwo Jun 10, 2024

Xuanwo Jun 10, 2024

evenyag commented Jun 11, 2024

evenyag commented Jun 11, 2024

Xuanwo Jun 12, 2024

Xuanwo Jun 12, 2024

evenyag Jun 12, 2024 •

edited

Loading

Xuanwo Jun 12, 2024

Xuanwo left a comment

	/// ## `chunk`
	///
	/// Set `chunk` for the writer.
	///
	/// OpenDAL is designed to write files directly without chunking by default, giving users
	/// control over the exact size of their writes and helping avoid unnecessary costs.
	///
	/// This is not efficient for cases when users write small chunks of data. Some storage services
	/// like `s3` could even return hard errors like `EntityTooSmall`. Besides, cloud storage services
	/// will cost more money if we write data in small chunks.
	///
	/// Set a good chunk size might improve the performance, reduce the API calls and save money.
	///
	/// The following example will set the writer chunk to 8MiB. Only one API call will be sent at
	/// `close` instead.

feat(core): sets default chunk_size and sends buffer > chunk_size directly #4710

feat(core): sets default chunk_size and sends buffer > chunk_size directly #4710

Conversation

evenyag commented Jun 7, 2024 • edited Loading

What's changed

Reference

evenyag commented Jun 7, 2024 • edited Loading

Xuanwo commented Jun 7, 2024

Xuanwo left a comment

Choose a reason for hiding this comment

Xuanwo Jun 10, 2024

Choose a reason for hiding this comment

Xuanwo Jun 10, 2024

Choose a reason for hiding this comment

evenyag commented Jun 11, 2024

evenyag commented Jun 11, 2024

Xuanwo Jun 12, 2024

Choose a reason for hiding this comment

Xuanwo Jun 12, 2024

Choose a reason for hiding this comment

evenyag Jun 12, 2024 • edited Loading

Choose a reason for hiding this comment

Xuanwo Jun 12, 2024

Choose a reason for hiding this comment

Xuanwo left a comment

Choose a reason for hiding this comment

evenyag commented Jun 7, 2024 •

edited

Loading

evenyag commented Jun 7, 2024 •

edited

Loading

evenyag Jun 12, 2024 •

edited

Loading